Add input validation against malformed inputs#293
Conversation
76c5d3e to
18243ab
Compare
9dc3932 to
23fb7d2
Compare
fd157f8 to
066c308
Compare
b274940 to
f4cd609
Compare
|
Updated PR description. I'd appreciate a human review following the suggestion from the PR description. 🤖 |
9c0398f to
f2a04f9
Compare
|
:fyi: the last commit (f2a04f9) is instructions only, no runtime change. It turns the validation approach into a documented convention: a Code Style rule in CLAUDE.md routing values by class (identifiers through Why: the pod name rules had already forked once while this branch was open (#344 grew its own validator), so this points new input surfaces at |
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
f2a04f9 to
8434e08
Compare
skyrpex
left a comment
There was a problem hiding this comment.
Looks good to me! The only thing that triggers my spider-sense is maybe building over-specific validation for generic inputs, for example:
Are we certain that a ResourceName will never contain .., /, ? or #?
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
|
Good spider-sense! 🕸️ Did some investigation on this, and confirmed that AWS has no universal resource-name grammar. S3 object keys can contain The validator in this PR only guards Cloud Pod names, and naming it For pod names we can be certain since the platform enforces I aligned the allow-list to that exact pattern, which also fixes a real over-restriction: we previously rejected leading I'd appreciate a second round when you have some time, @skyrpex! 🙏 |
Co-Authored-By: Claude <noreply@anthropic.com>
|
Pushed one more commit to fix the pipeline. |
Motivation
lstk is increasingly invoked by AI agents and scripts, which can produce malformed or hostile input (control characters, path traversal, percent-encoding, shell metacharacters) in ways humans rarely do. Pod names, the auth token, and
[env.*]config values were only loosely checked (or not at all), so bad input surfaced as a confusing error deep inside a Docker call or platform API request instead of failing fast with a clear reason.Changes
internal/validate, reusable deterministic validators (ResourceName,EnvVarName,NoControlChars,AuthToken) that return a machine-classifiable*Error(stableRulefield) instead of a generic message.validate.AuthTokenintocmd/root.go's token resolution (env/keyring): trims whitespace, then rejects control chars, embedded whitespace, or implausibly long tokens.validate.EnvVarName/NoControlCharsintointernal/config/config.goto reject malformed[env.*]keys/values before they're injected into a container's environment.internal/snapshot's bare pod-name regex withvalidate.ResourceName, adding deny-checks for percent-encoding, path traversal, embedded path/query characters, and shell metacharacters.mainhad independently grown its own pod-name validator (ValidatePodName, from the S3 remote-storage PR Add S3 remote storage for snapshot save, load, and list #344) that permits underscores and is also called directly for S3-remote pod names incmd/snapshot.go.ValidatePodNamenow delegates tovalidate.ResourceName, whose charset was widened to keep underscores working — the S3-remote pod-name path now gets the same deny-checks aspod:refs, which it didn't before.CLAUDE.md, and encode its usage as a convention in the agent instructions so future input surfaces route through it instead of growing parallel validators: a Code Style rule inCLAUDE.md(route by value class — identifiers →ResourceName; opaque secrets → looseAuthToken-style checks; paths/URLs → their existing parsers; extendinternal/validaterather than inlining a regexp), an input question + wiring/test steps in theadd-commandskill, and a checklist item in thereview-prskill.Review
Human review advised — changes user-facing error behavior for auth tokens, pod names, and
[env.*]config (previously-accepted values are now rejected with new/different messages), and this rebase merges two independently-evolved pod-name validators.Tests
New unit tests in
internal/validate/validate_test.go(all validators, deny-check ordering, rule codes) andinternal/config/env_validation_test.go; extendedinternal/snapshot/destination_test.gowith malformed pod-name cases (percent-encoding, embedded query, shell metacharacters) plus a regression case confirming underscore-named pods still pass. Verified after rebase:make build,go vet ./...,make test(full suite),make lint(both modules) all green.Closes PRO-306